Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Replace Timeout with RolloutConfig #281

Merged

Conversation

dhaiducek
Copy link
Member

Summary

Implement RolloutConfig, which provides more fine-tuned configuration for users than the current Timeout.

Related issue(s)

@qiujian16
Copy link
Member

@dhaiducek since we are about to release 0.12.0, would like to know the progress of this PR. Is it ready for review, do you plan to include it in 0.12.0 or it can be moved to next release?

@dhaiducek
Copy link
Member Author

@qiujian16 It was waiting on #276, but now that it's merged I'll continue work here. I'll work on it this week, but I think it's also fine if v0.12.0 is released without these library updates so that this PR doesn't block the release.

@dhaiducek
Copy link
Member Author

Status update: I wrestled with the CI a bit because I'm on an M1 MacBook and some of the binaries aren't actually built for arm64 (those changes are in this PR). I've got current code working, migrating Timeout --> ProgressDeadline, but need to add logic (and tests) for the other fields that are in the new struct alongside it.

@dhaiducek dhaiducek marked this pull request as ready for review November 2, 2023 20:20
@openshift-ci openshift-ci bot requested review from deads2k and qiujian16 November 2, 2023 20:20
@dhaiducek dhaiducek changed the title [WIP] ✨ Replace Timeout with RolloutConfig ✨ Replace Timeout with RolloutConfig Nov 2, 2023
@dhaiducek
Copy link
Member Author

/cc @serngawy @haoqing0110

Sorry for the long delay here--between wrestling with the CI and getting pulled away for other tasks, it's taken longer to update than I thought. There might be some more tests required here, but I think the logic is largely (if not entirely) there.

Some notes:

  • I added logic to pass Timeout over to ProgressDeadline and mark Timeout as deprecated. I debated having MaxFailures set to 100% so the behavior wouldn't change there, but I think it better to leave it and make the migration obvious.
  • I made a decision to handle MaxFailures over the total number of clusters for Progressive and per group for ProgressivePerGroup, partially because Progressive clusters are sorted alphabetically, so having a cluster injected at the beginning or middle could complicate things.

@serngawy
Copy link
Member

serngawy commented Nov 3, 2023

Sounds good to me in general, I added more unit test to cover Rollout ProgressivePerGroup case.

@dhaiducek
Copy link
Member Author

After discussion with @serngawy I've updated the logic so that MandatoryDecisionGroups tolerate no failures for any progressive rollout type.

@haoqing0110
Copy link
Member

LGTM, seems the ProgressDeadline is just a rename of Timeout, I'm fine to leave the Timeout and mark it as deprecated.

Uses `MinSuccessTime`, `ProgressDeadline`, and `MaxFailures` for a more
fine-tuned configuration.
Also passes `Timeout` over to `ProgressDeadline` and marks it as
deprecated.

Signed-off-by: Dale Haiducek <[email protected]>
serngawy and others added 2 commits November 6, 2023 16:44
- MandatoryDecisionGroups should tolerate no failures
- The logic for ProgressivePerGroup was incorrect

Signed-off-by: Dale Haiducek <[email protected]>
@qiujian16
Copy link
Member

/approve

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

@dhaiducek
Copy link
Member Author

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

Sounds good, thanks @qiujian16!

@serngawy @haoqing0110 I'd appreciate if you'd do another review here when you have a moment! 😄

@serngawy
Copy link
Member

serngawy commented Nov 9, 2023

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

Sounds good, thanks @qiujian16!

@serngawy @haoqing0110 I'd appreciate if you'd do another review here when you have a moment! 😄

I gave it another review, sounds good to me @dhaiducek appreciate the good work :)

ops, didn't consider my approve will merge it @haoqing0110

Copy link
Contributor

openshift-ci bot commented Nov 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, qiujian16, serngawy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants